8365919: Replace currentTimeMillis with nanoTime in Stresser.java #26879

albertnetymk · 2025-08-21T10:02:12Z

Mostly a mechanic change from currentTimeMillis() to nanoTime()/1000000. It also removes finishTime to avoid overflowing.

The change in iteration() is to ensure currentTime is properly initialized.

(Was investigating a timeout on Windows-x64, which led me to this code. I think this fix is good enough by its own.)

Test: tier1-5

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8365919: Replace currentTimeMillis with nanoTime in Stresser.java (Enhancement - P4)

Reviewers

Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26879/head:pull/26879
$ git checkout pull/26879

Update a local copy of the PR:
$ git checkout pull/26879
$ git pull https://git.openjdk.org/jdk.git pull/26879/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26879

View PR using the GUI difftool:
$ git pr show -t 26879

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26879.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-21T10:03:21Z

👋 Welcome back ayang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-08-21T10:04:18Z

@albertnetymk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8365919: Replace currentTimeMillis with nanoTime in Stresser.java

Reviewed-by: phh

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

040cc7a: 8365292: Remove javax.imageio.spi.ServiceRegistry.finalize()
d24449f: 8365815: JFR: Update metadata.xml with 'jfr query' examples
45726a1: 8365052: G1: Remove G1CollectionSet::groups() accessors
... and 14 more: https://git.openjdk.org/jdk/compare/ae0dac43c09377c87e9b0452618a5b32c8568150...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-08-21T10:05:07Z

@albertnetymk To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
graal
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
net
nio
security
serviceability
shenandoah

albertnetymk · 2025-08-21T10:11:50Z

/cc hotspot-gc

openjdk · 2025-08-21T10:12:54Z

@albertnetymk
The hotspot-gc label was successfully added.

mlbridge · 2025-08-21T10:16:03Z

Webrevs

phohensee

You might want to load (options.getTime() * 1000) into a local rather than compute it multiple times.

albertnetymk · 2025-08-22T18:38:15Z

You might want to load (options.getTime() * 1000) into a local rather than compute it multiple times.

Extracted that to a local var for getTimeLeft. I skipped printExecutionInfo, as I feel introducing a local var there actually makes it a bit harder to read.

tschatzl · 2025-08-25T12:03:47Z

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java

@@ -179,15 +181,15 @@ public void printStressInfo(PrintStream out) {
     */
    public void printExecutionInfo(PrintStream out) {
        println(out, "Completed iterations: " + iterations);
-        println(out, "Execution time: " + (currentTime - startTime) + " seconds");
+        println(out, "Execution time: " + (currentTime - startTime)/1000.0 + " seconds");


There should be spaces around the operator. The change sometimes has them and sometimes not.

Suggested change

println(out, "Execution time: " + (currentTime - startTime)/1000.0 + " seconds");

println(out, "Execution time: " + (currentTime - startTime) / 1000.0 + " seconds");

tschatzl · 2025-08-25T12:08:28Z

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java

@@ -318,11 +317,12 @@ public long getExecutionTime() {
     * @return time
     */
    public long getTimeLeft() {
-        long current = System.currentTimeMillis();
-        if (current >= finishTime) {
+        long elapsedTime = System.nanoTime()/1000000 - startTime;


This could maybe use getExecutionTime()? Also the repeated use of the term System.nanoTime() / 1000000 five time definitely warrants a helper function imo, and allows documentation of the reason for its use.

tschatzl · 2025-08-25T12:16:35Z

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java

-        } else {
-            finishTime = startTime + stressTime * 1000;
-        }
+        startTime = System.nanoTime()/1000000;


If the reason for the change is to use System.nanoTime() instead of System.currentTimeMillis() because of monoticity, wouldn't it be simpler to add a helper that returns a millisecond value based on System.nanoTime() and call that one instead of System.currentTimeMillis() and keep everything else the same?

Inlining the System.nanoTime() / 1000000 statement a few times seems bad. Having a helper function for this might allow to include a comment why System.currentTimeMillis() is not used too.

Or use nanoTime resolution throughout the calculations and only normalize on printout and when comparing with the deadline. Although nanoTime is here not used for its resolution, I find it quite unusual to see that /1_000_000 sprinkled around the start time recording.

tschatzl · 2025-08-25T12:23:02Z

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java

        if (startTime == 0) {
            throw new TestBug("Stresser is not started.");
        }
        return !forceFinish
                && !finished
                && (maxIterations == 0 || iterations < maxIterations)
-                && (finishTime == 0 || currentTime < finishTime);
+                && (options.getTime() == 0 || (currentTime - startTime) < options.getTime() * 1000);


I doubt that inlining options.getTime() * 1000 literally 10 times instead of storing it in a local (finishTime) or having a getter makes the code easier to understand.

Maybe just rename finishTime to sth like stressTime?

albertnetymk · 2025-08-25T14:09:11Z

I have changed to use nanoseconds for all fields, to avoid excessive unit conversion.

test-nano-time

7af0923

openjdk bot changed the title ~~JDK-8365919~~ 8365919: Replace currentTimeMillis with nanoTime in Stresser.java Aug 21, 2025

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Aug 21, 2025

openjdk bot added the rfr Pull request is ready for review label Aug 21, 2025

phohensee reviewed Aug 22, 2025

View reviewed changes

albertnetymk added 2 commits August 22, 2025 20:29

Merge branch 'master' into test-nano-time

5e9c7a9

review

cefb994

tschatzl suggested changes Aug 25, 2025

View reviewed changes

review

10404ce

phohensee approved these changes Aug 25, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8365919: Replace currentTimeMillis with nanoTime in Stresser.java #26879

8365919: Replace currentTimeMillis with nanoTime in Stresser.java #26879

albertnetymk commented Aug 21, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Aug 21, 2025

Uh oh!

openjdk bot commented Aug 21, 2025 •

edited

Loading

Uh oh!

openjdk bot commented Aug 21, 2025

Uh oh!

albertnetymk commented Aug 21, 2025

Uh oh!

openjdk bot commented Aug 21, 2025

Uh oh!

mlbridge bot commented Aug 21, 2025 •

edited

Loading

Uh oh!

phohensee left a comment

Uh oh!

albertnetymk commented Aug 22, 2025

Uh oh!

tschatzl Aug 25, 2025

Uh oh!

tschatzl Aug 25, 2025

Uh oh!

tschatzl Aug 25, 2025

Uh oh!

ecki Aug 25, 2025

Uh oh!

tschatzl Aug 25, 2025

Uh oh!

albertnetymk commented Aug 25, 2025

Uh oh!

Uh oh!

	println(out, "Execution time: " + (currentTime - startTime)/1000.0 + " seconds");
	println(out, "Execution time: " + (currentTime - startTime) / 1000.0 + " seconds");

8365919: Replace currentTimeMillis with nanoTime in Stresser.java #26879

Are you sure you want to change the base?

8365919: Replace currentTimeMillis with nanoTime in Stresser.java #26879

Conversation

albertnetymk commented Aug 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 21, 2025

Uh oh!

openjdk bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 21, 2025

Uh oh!

albertnetymk commented Aug 21, 2025

Uh oh!

openjdk bot commented Aug 21, 2025

Uh oh!

mlbridge bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

phohensee left a comment

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Aug 22, 2025

Uh oh!

tschatzl Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

ecki Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Aug 25, 2025

Uh oh!

Uh oh!

albertnetymk commented Aug 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 21, 2025 •

edited

Loading

mlbridge bot commented Aug 21, 2025 •

edited

Loading